BranchHinting fuzzing: Do not remove effects when deinstrumenting#8613
BranchHinting fuzzing: Do not remove effects when deinstrumenting#8613kripken merged 3 commits intoWebAssembly:mainfrom
Conversation
tlively
left a comment
tlively
left a comment
There was a problem hiding this comment.
Have we considered having the branch log function return the condition value so we don't need to introduce new locals to use it? That would make removing it trivial as well.
| (if | ||
| (i32.const 1) | ||
| (then | ||
| (unreachable) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Can this be simplified to just (unreachable)?
There was a problem hiding this comment.
Unfortunately no, then all of it would have type unreachable, which is handled in another way.
| struct DeInstrumentBranchHints | ||
| : public InstrumentationProcessor<DeInstrumentBranchHints> { | ||
|
|
||
| template<typename T> void processCondition(T* curr) { |
There was a problem hiding this comment.
Maybe it's better to comment on what happens to the code that has not been swapped here due to side effects..? (It looks like logging calls are removed at the end)
By the way why do we need this replacement when we remove logging calls at the end anyway? (If it's for removing unnecessary code, wouldn't running the normal optimizer remove them?)
There was a problem hiding this comment.
Good idea, added a comment now, also to clarify your question. Removing the calls is necessary, while the other cleanup, that sometimes we can't do, is mainly for easy debugging.
Might be worth trying that. I don't see an obvious reason why it wouldn't work (but it's been a while since I worked on this). |
3 participants
The BranchHinting fuzzer broke after #8608, but it wasn't that PR's fault -
just that we now generate a different pattern of blocks that BranchHinting's
deinstrumentation did not handle yet. The problem was that the block with
the condition that we replace now has effects, so we can't always remove it
wholesale.